Fix incorrect slice read in blocks #474
Conversation
targets a TypeError "TypeError: Cannot read properties of undefined (reading 'offset')" that was happening when: - a slice read was equal to a block size - reading last part of the data The fix removes the "one-off" bug: - top is the first byte of the next block that we don't want to read. - The last block we want to read (blockIdHigh) is found by knowing the last byte we want to read and divide it by the block size. - Moved blockedSource declaration inside each test for better isolation. (BlockedSource is stateful)
| } | ||
| const blockIdLow = Math.floor(slice.offset / this.blockSize); | ||
| const blockIdHigh = Math.floor(top / this.blockSize); | ||
| const blockIdHigh = Math.floor((top - 1) / this.blockSize); |
There was a problem hiding this comment.
The PR mention the bug was occurring when reading the last block and the blockSize is equals to slice.length.
For what I see, this change may also affect the behavior of reading the first block.
In a situation where slice.offset is 0 and blockSize is equals to slice.length, for the previous implementation, blockIdHigh would have a value of 1, with the new implementation it will have a value of 0.
There was a problem hiding this comment.
that's a great observation! It is a bug as well. It was documented with a test at https://github.com/geotiffjs/geotiff.js/pull/474/files#diff-492ba43e11ce778a6c52157c7795c3a45b40213fb11b00df31dbb469614b4bd1R1325-R1335
In case you describe, our data would fit exactly in one block - blockId 0.
There was a problem hiding this comment.
I've generalized this case in the description as reading a block that is equal to a block size (so originally the code would also fail if you try to read offset 2, blockSize 2 and slice length 2.. etc.)
|
Is there anything else needed to merge this PR? |
|
What else is needed? |
There was a problem hiding this comment.
LGTM, although I haven't dug too deep into this bug or the BlockedSource itself. I have more of a question than comment about one test, but otherwise this worked for me in quick npm pack downstream check.
Edit: I've since dug a bit deeper with this and confirmed that this PR does in fact resolve #374. I think even without any fixes, the tests included here are valuable. On our current master the following tests fail:
Fetches all data in a single block
Fetches complete first block
Fetches complete last block
Fetches partial data from the end
but they pass here, which is a pretty good demonstration of the actual issue at play here.
I'm still wrapping my head around when this might come into play with real world usage, and trying to find or create reliable GeoTIFF data to test with.
What
Addresses: #374
targets a TypeError "TypeError: Cannot read properties of undefined (reading 'offset')" that was happening when:
Details
The fix removes the "one-off" bug:
Test improvements: